Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Header titles added #2042

Merged
merged 23 commits into from
Apr 1, 2022
Merged

Header titles added #2042

merged 23 commits into from
Apr 1, 2022

Conversation

tanmoyAtb
Copy link
Contributor

closes #1611


Submission checklist:
No UI changes

@render
Copy link

render bot commented Mar 24, 2022

@render
Copy link

render bot commented Mar 24, 2022

@render
Copy link

render bot commented Mar 24, 2022

@github-actions github-actions bot added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label Mar 24, 2022
@tanmoyAtb tanmoyAtb marked this pull request as ready for review March 25, 2022 06:18
@asnaith
Copy link
Member

asnaith commented Mar 25, 2022

There wasn't too much for me to test here but I did run through the pages listed on the issue on both desktop / mobile and everything visually looked as expected :)

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are missing translation for the title, I just commented for some of them, but we'd need them for all.

@tanmoyAtb
Copy link
Contributor Author

We are missing translation for the title, I just commented for some of them, but we'd need them for all.

Thanks for that, Added the translation tags, I left out "Chainsafe Files" part for translations as we've kept it that way everywhere else

@tanmoyAtb tanmoyAtb enabled auto-merge (squash) March 29, 2022 12:38
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good but when I logged into Files, on the render preview, I got an error:

Helmet expects a string as a child of <title>. Did you forget to wrap your children in braces? ( <title>{``}</title> ) Refer to our API for more information.

I guess the Trans wasn't expected there. so we probably need {${ttitle} - Chainsafe Files} or something along this line

@tanmoyAtb
Copy link
Contributor Author

Code looks good but when I logged into Files, on the render preview, I got an error:

Helmet expects a string as a child of <title>. Did you forget to wrap your children in braces? ( <title>{``}</title> ) Refer to our API for more information.

I guess the Trans wasn't expected there. so we probably need {${ttitle} - Chainsafe Files} or something along this line

Thanks for saving this one, updates are in.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested Files and it looks good.

@tanmoyAtb tanmoyAtb disabled auto-merge April 1, 2022 06:00
@tanmoyAtb tanmoyAtb enabled auto-merge (squash) April 1, 2022 09:38
@tanmoyAtb tanmoyAtb merged commit 2927d6f into dev Apr 1, 2022
@tanmoyAtb tanmoyAtb deleted the mnt/page-header-titles-1611 branch April 1, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add headers to pages
5 participants